PS-1576: Add new QGetSharedUrl method #74
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new QGetSharedUrl function to the flipAPI package that allows users to retrieve shareable URLs for files uploaded to the Displayr Cloud Drive. This functionality is useful for building external reports automatically from Displayr.
Changes:
- Added
QGetSharedUrlfunction that makes a POST request to the DataMart/Share API endpoint to get or create a share URL for a specified file
R/DataMart.R
Outdated
|
|
||
| #' Share an object | ||
| #' | ||
| #' Get's the cloud drive share url for an object saved to the Displayr Cloud Drive. |
There was a problem hiding this comment.
Typo in documentation: "Get's" should be "Gets" (no apostrophe). The apostrophe indicates possession or a contraction, not a simple third-person singular verb.
| #' Get's the cloud drive share url for an object saved to the Displayr Cloud Drive. | |
| #' Gets the cloud drive share url for an object saved to the Displayr Cloud Drive. |
R/DataMart.R
Outdated
| stop("QSaveData has encountered an unknown error. ", | ||
| "404: No such file exists. ", | ||
| "The likely cause was an incorrect path preceding the filename, or insufficient access to the file path.") | ||
| } | ||
| else if (has.errored || res$status_code != 200) | ||
| { | ||
| warning("QSaveData has encountered an unknown error.") |
There was a problem hiding this comment.
The error messages reference "QSaveData" instead of "QGetSharedUrl". This will confuse users when they encounter errors from this function. Update all error messages to reference the correct function name.
| stop("QSaveData has encountered an unknown error. ", | |
| "404: No such file exists. ", | |
| "The likely cause was an incorrect path preceding the filename, or insufficient access to the file path.") | |
| } | |
| else if (has.errored || res$status_code != 200) | |
| { | |
| warning("QSaveData has encountered an unknown error.") | |
| stop("QGetSharedUrl has encountered an unknown error. ", | |
| "404: No such file exists. ", | |
| "The likely cause was an incorrect path preceding the filename, or insufficient access to the file path.") | |
| } | |
| else if (has.errored || res$status_code != 200) | |
| { | |
| warning("QGetSharedUrl has encountered an unknown error.") |
R/DataMart.R
Outdated
| stop("QSaveData has encountered an unknown error. ", | ||
| "404: No such file exists. ", | ||
| "The likely cause was an incorrect path preceding the filename, or insufficient access to the file path.") | ||
| } | ||
| else if (has.errored || res$status_code != 200) | ||
| { | ||
| warning("QSaveData has encountered an unknown error.") |
There was a problem hiding this comment.
The warning message references "QSaveData" instead of "QGetSharedUrl". This will confuse users when they encounter this warning from this function. Update the warning message to reference the correct function name.
| stop("QSaveData has encountered an unknown error. ", | |
| "404: No such file exists. ", | |
| "The likely cause was an incorrect path preceding the filename, or insufficient access to the file path.") | |
| } | |
| else if (has.errored || res$status_code != 200) | |
| { | |
| warning("QSaveData has encountered an unknown error.") | |
| stop("QGetSharedUrl has encountered an unknown error. ", | |
| "404: No such file exists. ", | |
| "The likely cause was an incorrect path preceding the filename, or insufficient access to the file path.") | |
| } | |
| else if (has.errored || res$status_code != 200) | |
| { | |
| warning("QGetSharedUrl has encountered an unknown error.") |
R/DataMart.R
Outdated
| has.errored <- inherits(res, "try-error") | ||
|
|
||
| if (!has.errored && res$status_code == 413) # 413 comes from IIS when we violate its web.config limits | ||
| stopBadRequest(res, "Could not write to Displayr Cloud Drive. Data to write is too large.") |
There was a problem hiding this comment.
The error message context is incorrect. The 413 status code indicates that the request is too large, but this function doesn't write data to the Cloud Drive - it only shares an existing file. The error message should be updated to reflect the actual operation being performed (e.g., "Could not share file. Request is too large.").
| stopBadRequest(res, "Could not write to Displayr Cloud Drive. Data to write is too large.") | |
| stopBadRequest(res, "Could not share file. Request is too large.") |
There was a problem hiding this comment.
Just removing this, this isn't possible.
R/DataMart.R
Outdated
| content <- httr::content(res) | ||
| # The content returns a JSON object with the share url in the 'sharingUrl' field | ||
| # and a boolean (that we ignore) that indicates if the file was newly shared or not | ||
| return (content$sharingUrl) |
There was a problem hiding this comment.
The function uses explicit return() for the final return value, which violates the coding guideline to avoid redundant returns. The last evaluated expression is returned by default. Remove the return() and just use content$sharingUrl.
R/DataMart.R
Outdated
| #' @param filename character string. Name of the file that is being shared. | ||
| #' To reference a file in a subdirectory, use double backslashes after each folder (e.g "subdir\\file.csv"). | ||
| #' | ||
| #' @importFrom httr POST add_headers upload_file |
There was a problem hiding this comment.
The import upload_file is declared but not used in this function. Remove it from the @importFrom directive.
| #' @importFrom httr POST add_headers upload_file | |
| #' @importFrom httr POST add_headers |
R/DataMart.R
Outdated
| #' @param filename character string. Name of the file that is being shared. | ||
| #' To reference a file in a subdirectory, use double backslashes after each folder (e.g "subdir\\file.csv"). | ||
| #' | ||
| #' @importFrom httr POST add_headers upload_file |
There was a problem hiding this comment.
Missing @importFrom directive for httr content. The function uses httr::content(res) on line 425 but doesn't import it. Add @importFrom httr content to the documentation.
| #' @importFrom httr POST add_headers upload_file | |
| #' @importFrom httr POST add_headers upload_file content |
roman-polak
left a comment
roman-polak
left a comment
There was a problem hiding this comment.
This looks good, but I think package version should be bumped up in DESCRIPTION.
This allows a user to get the shareable url for a given uploaded datamart object.
Very useful when building external reports automatically from Displayr.